Align SqlException Numbers across platforms #3461
Merged
+146
−47
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
campersau
reviewed
Jul 4, 2025
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniError.netcore.cs
Outdated
Show resolved
Hide resolved
paulmedynski
requested changes
Jul 4, 2025
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs
Show resolved
Hide resolved
paulmedynski
previously approved these changes
Jul 7, 2025
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniError.netcore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs
Show resolved
Hide resolved
paulmedynski
previously approved these changes
Jul 10, 2025
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniError.netcore.cs
Outdated
Show resolved
Hide resolved
paulmedynski
previously approved these changes
Jul 11, 2025
|
Let's resolve the conflicts and promote from draft to normal. |
- Better capture error scenarios in TCP managed SNI. - Fix logging bug in SqlClientEventSource. - Change nativeError from uint to int
paulmedynski
force-pushed
the
dev/david/cross-plat-error-nums
branch
from
f76098c to
6d0993c
Compare
|
I rebased to main, fixed the conflicts, and squashed commits. Please re-review. |
paulmedynski
requested changes
Jul 11, 2025
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs
Outdated
Show resolved
Hide resolved
|
Will this be a breaking change? |
benrr101
previously approved these changes
Jul 11, 2025
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs
Outdated
Show resolved
Hide resolved
paulmedynski
previously approved these changes
Jul 12, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3461 +/- ##
==========================================
- Coverage 68.86% 66.02% -2.84%
==========================================
Files 280 276 -4
Lines 62417 62216 -201
==========================================
- Hits 42982 41079 -1903
- Misses 19435 21137 +1702
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
paulmedynski
approved these changes
Jul 14, 2025
We don't think so, but I will check the updated code paths to see if anything sticks out. These changes improve surfacing of underlying connection error codes on non-Windows, which boils down to exposing existing specific error codes at the appropriate times. |
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs
Show resolved
Hide resolved
|
So it will not affect code relying on certain values to be thrown? |
benrr101
approved these changes
Jul 14, 2025
The affected scenarios are all currently surfacing an error number of "0", which is very generic and arguably not actionable. This change should provide actionable error numbers. Linked issues generally refer to this as a "bug" since the underlying errors are actually being hidden. While you could argue that this behavior change is breaking, we've decided to take it for 6.1 since 6.1 will likely be a LTS release and we would be less inclined to make a behavior change in a hotfix. The behavior change will definitely be documented in the release notes. |
samsharma2700
approved these changes
Jul 14, 2025
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs
Show resolved
Hide resolved
paulmedynski
added a commit
that referenced
this pull request
Jul 15, 2025
* - Align SqlException Numbers across platforms - Better capture error scenarios in TCP managed SNI. - Fix logging bug in SqlClientEventSource. - Change nativeError from uint to int * - Removed duplicate SniErrorDetails object and aligned field names. * - Added localized string for the new connection timed out exception. * - Fixed tests sensitive to OS newlines. --------- Co-authored-by: Paul Medynski <31868385+paulmedynski@users.noreply.github.com>
paulmedynski
added a commit
that referenced
this pull request
Jul 16, 2025
* Reduce automated test crashes (#2968) * Converted Threads to long-running Tasks The key advantage is that exceptions propagate properly. If a thread throws an exception (as a result of a failed test assertion, or otherwise) then the test host crashes and must be restarted. * Corrected the instantiation of the cancellation task - missing state parameter. * Changes to TestSqlCommandCancel, eliminating timing-specific cancellation behaviour testing. This should also allow the test to run on both netcore and netfx. * Responding to code review. * Removed two unnecessary iterations from DatabaseHelper. * Added explanatory comments to ApiShould. * Switched to using Task.WaitAll rather than waiting for each Task in sequence. * Improve cancellation detection Cancellation can trigger one of several different errors, resulting in a flakier test. Also ensure that the query always takes more than 150ms, ensuring that a quick query execution doesn't cause the test to fail. Finally, make sure that we try to read everything from the SqlDataReader. * Correcting previous merge * Align SqlException Numbers across platforms (#3461) * - Align SqlException Numbers across platforms - Better capture error scenarios in TCP managed SNI. - Fix logging bug in SqlClientEventSource. - Change nativeError from uint to int * - Removed duplicate SniErrorDetails object and aligned field names. * - Added localized string for the new connection timed out exception. * - Fixed tests sensitive to OS newlines. --------- Co-authored-by: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> * Align SqlException Numbers across platforms - Manually fixing some error reporting that wasn't cherr-picked because it was part of larger changes. * Updated TdsParser.Windows.cs to use the SniErrorDetails struct from TdsParserStateObject. --------- Co-authored-by: Edward Neal <55035479+edwardneal@users.noreply.github.com> Co-authored-by: David Engel <davidengel@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Attempt to better align SqlException Numbers across platforms. Wondering if it might be this simple.
Issues
#1773